Skip to content

Hold native delivery while a Codex thread waits on a human - #274

Merged
schickling merged 1 commit into
mainfrom
schickling/fix-265-active-flags
Aug 18, 2026
Merged

Hold native delivery while a Codex thread waits on a human#274
schickling merged 1 commit into
mainfrom
schickling/fix-265-active-flags

Conversation

@schickling

@schickling schickling commented Aug 18, 2026

Copy link
Copy Markdown
Contributor

Why

ThreadStatus's active arm carries a required activeFlags: ThreadActiveFlag[], where
ThreadActiveFlag is exactly "waitingOnApproval" | "waitingOnUserInput". observe_thread_status
took status: &str — one word — so all three call sites (:702, :724, :732) read
.../status/type and discarded the rest of the status object at the parse site. st2 therefore could
not distinguish "the model is working" from "the model has stopped and is waiting for a human".

The delivery consequence follows from st2's own code: an approval happens mid-turn, so the session is
Active { turn_id }, and maybe_request maps Active to CodexDeliveryMethod::Steer
(:415-419). st2 steers a [DING] into a session that is sitting on an approval dialog.
That consequence is inferred, not observed — see ## Limits.

What

One localized change to observe_thread_status and its three call sites.

  • Parse the field. New human_blocking_flag(status: Option<&Value>) -> Option<CodexHoldReason>
    reads activeFlags from the status object and returns the first flag that means "blocked on a
    human". It is called at all three sites, passing the status object each already had in hand.
  • Map it onto an existing hold. Two CodexHoldReason variants, WaitingOnApproval and
    WaitingOnUserInput. A flag arriving while Active { turn_id } becomes
    Held { reason, turn_id: Some(turn_id) }.
  • Release the same turn when the flag clears. {"type":"active","activeFlags":[]} arriving on a
    flagged hold returns to Active { turn_id }. This is the case a naive fix gets wrong: the old
    "active" arm preserved state only for Active and Held{Review|Compaction|ConflictingTurn}, so
    a cleared flag would have fallen through to Held{ActiveWithoutTurn, turn_id: None} — turn ID
    lost, and no second turn/started arrives mid-turn to restore it. Native delivery would have
    stalled for the rest of the turn.
  • Unknown flag values degrade to plain active, and a missing or malformed array reads as no
    flag rather than failing the frame. observe's error propagates via ? (:1944), so a strict
    accessor on a schema surprise would kill the control watcher.
  • A more specific hold still outranks the flag. Review, Compaction and ConflictingTurn
    keep their own turn IDs, which the turn-lifecycle handlers depend on.

maybe_request is not modified. It already returns None for every Held reason
(:420-421), so the behavioural half is one more input mapped onto the existing, invariant-pinned
hold path — the same path Review and Compaction take.

Out of scope and untouched: observe_turn_completed (#264), the item/started / item/completed
handling (#266), and SUPPORTED_CODEX_CLI_VERSIONS (#267).

Impact

st2 declines to steer into a Codex session that has stopped for a human, and resumes steering the
same turn once the human answers.

Why this is fail-closed, not fail-open. Declining is not dropping. A hold makes maybe_request
return None, which is the already-tested behaviour for Review and Compaction
(review_compaction_and_dnd_hold_the_unread_fifo_head): the unread FIFO head stays in the inbox, no
delivery ownership is recorded, no paste or archive happens, and the same head is delivered on the
next poll once the state clears. The new test asserts exactly that round trip — held twice with the
message still on disk, then turn/steer with expectedTurnId: turn-1 after release. The failure
direction the change introduces is deferral; the failure direction it removes is an unobservable
steer into a modal. No new delivery path is created and no Delivered classification is relaxed.

One deliberate edge, pinned but not fixed here: a turn that completes while still flagged falls into
the pre-existing ConflictingTurn catch-all in observe_turn_completed and is released by the next
idle status. That function is #264's region, so it was read, not edited; the test asserts only that
the flagged state cannot decay into a steerable turn.

INVARIANTS.md is unchanged. Per the issue's "or is documented as deliberately unpinned" branch,
the steerability rule stays a code comment (:160-163): a single hold-reason mapping is not the tier
CLAUDE.md reserves rows for, and INVARIANTS.md is a shared file three sibling PRs are near. The
local proof is the named test below.

Checks

Schema evidence, generated locally against the two versions st2 actually supports:

# 0.145.0 and 0.146.0 installed into throwaway trees; each tree's own binary was version-checked
# before generating, so the schema is attributed to the version that actually emitted it.
npm install @openai/codex@0.145.0        # ./node_modules/.bin/codex --version -> codex-cli 0.145.0
npm install @openai/codex@0.146.0        # ./node_modules/.bin/codex --version -> codex-cli 0.146.0
./node_modules/.bin/codex app-server generate-json-schema --out schema145   # from the 0.145.0 tree
./node_modules/.bin/codex app-server generate-json-schema --out schema146   # from the 0.146.0 tree
codex app-server generate-json-schema --out schema147                       # installed 0.147.0

v2/ThreadStatusChangedNotification.json → ThreadStatus.ActiveThreadStatus:
  0.145.0  required: ["activeFlags", "type"]   activeFlags: array<ThreadActiveFlag>
  0.146.0  required: ["activeFlags", "type"]   activeFlags: array<ThreadActiveFlag>
  0.147.0  required: ["activeFlags", "type"]   activeFlags: array<ThreadActiveFlag>
  ThreadActiveFlag = enum ["waitingOnApproval", "waitingOnUserInput"]   (identical on all three)

activeFlags is present, required, and identically shaped on 0.145.0 and 0.146.0 — the complete
SUPPORTED_CODEX_CLI_VERSIONS set. The issue's "unverified on a supported version" caveat is
retired; #267 is unaffected either way, and :38 is untouched.

Test, written before the fix and confirmed failing against it:

nix develop --command cargo test --locked --lib waiting_on_a_human

  before (variants added, no parse):
    panicked at src/codex_app_server.rs:3916:
    assertion failed: state.observe(&status_changed(flags)).unwrap()
    → the frame changed nothing; st2 stayed Active{turn-1} and would have steered

  after:
    test codex_app_server::tests::
      waiting_on_a_human_holds_the_exact_turn_and_releases_it_when_the_flag_clears ... ok

It covers the four cases the issue lists — [], ["waitingOnApproval"], ["waitingOnUserInput"],
an unknown value — plus a mixed array, thread/started carrying the field before any turn is known,
a status arm with no activeFlags at all, the clear-and-release round trip, and the delivery
assertion.

Delivery path, since the behavioural half is included:

nix develop --command cargo test --locked --lib ding::
  → 56 passed; 0 failed

  All 18 tests named in the Fail-closed observed native DING row of INVARIANTS.md are in that
  run and green, including poke_text_normalizes_and_bounds_untrusted_fields,
  pty_delivery_uses_face607_delay_order_and_seconds,
  successful_transport_with_retained_or_unproven_pixels_is_not_delivered,
  staged_ownership_survives_archive_and_never_repastes,
  archived_not_retained_releases_fifo_without_repasting_owned_notice, and
  pending_delivery_ignores_busy_but_respects_fresh_dnd_archive_and_retry.

nix develop --command cargo test --locked --lib codex_app_server
  → 35 passed; 0 failed
  Includes control_initializes_before_recording_the_first_thread_only, the existing
  per-direction request-ID test that injects item/commandExecution/requestApproval (:3214).

Full suite, honestly reported:

nix develop --command cargo test --locked --no-fail-fast
  lib: 322 passed; 0 failed   (321 on the base commit, +1 new test)
  13 integration tests fail.

The same 13 fail identically on the unmodified base commit f177520 in this worktree, so the sets
are equal and this change adds none of them. They are environment- and concurrency-bound (real PTY,
systemd scopes, cgroups, and sibling agents running the full suite on the same machine):
classification_only_and_nested_agent_filename_changes_are_exact,
canonical_agents_freeze_the_admitted_route_across_post_boot_catalog_mutation,
clean_path_supports_help_validate_env_and_doctor,
exec_task_survives_transport_cgroup_cascade, managed_agent_color_contract_crosses_systemd_scope,
pty_task_survives_transport_cgroup_cascade, st2_down_tears_down_a_spec_fleet,
st2_up_boots_a_specs_team, st2_up_once_atomically_respawns_a_hard_killed_agent,
st2_up_spec_supervises_and_respawns_a_killed_agent,
targeted_once_real_pty_preserves_sibling_generation_across_selected_lifecycle,
tracked_product_surface_contains_only_native_names,
up_materialize_only_writes_the_overlay_without_needing_pty.

cargo fmt --check is dirty repo-wide on main (438 diffs). Every hunk this branch adds is
rustfmt-clean; the remaining diffs in src/codex_app_server.rs are at lines this branch does not
touch. No Darwin run.

Limits

  • The consequence is inferred, not observed. No capture has ever shown a populated
    activeFlags. The one live attempt saw "activeFlags":[] because the account hit its ChatGPT
    usage limit before an approval was reachable, and a re-attempt on this branch hit the same limit —
    the error payload names 2026-08-20 as the reset. That a session sitting on an approval prompt
    reports active with ["waitingOnApproval"] follows from the enum's names and the field's
    placement inside the active arm. It is schema evidence, not wire evidence. Nothing in this PR
    should be read as a claim that the steer-into-dialog behaviour was observed.
  • Version caveat, weakened but not silent. The schema check above is a real check on both
    supported versions, so the field is no longer "unverified on a supported version". What remains
    unverified is that 0.145.0 and 0.146.0 populate it in the situation the flag names — the same
    gap as the point above, on every version. The field is required on ActiveThreadStatus on all
    three, so its presence is not in question.
  • Unknown flag values stay steerable. This is the issue's explicit requirement, and it is a
    fail-open edge: a future codex-cli flag meaning "blocked" would read as plain active until
    someone classifies it. The control is the version gate — the comment at :35-37 requires a
    delivery-critical schema comparison before any new version is admitted, and that comparison is
    where a new flag value has to be classified. Erring the other way would let one new benign flag
    silently stop native delivery fleet-wide.
  • activeFlags is persisted as a derived hold, not as the raw array. The delivery-relevant
    content of the array is whether a known human-blocking flag is present, and the hold reason names
    which one. If both flags are ever set at once, only the first is reported.
  • Staggered binary upgrade. CodexControlState is #[serde(deny_unknown_fields)] (:192) and
    no field is added here, but two CodexHoldReason variants are. An older binary reading a record
    containing "reason":"waitingOnApproval" fails serde_json::from_slice in
    load_current_control_state (:2231) — an unknown enum variant is a hard error, not an ignored
    key, and the schema string st2.codex-control-state.v1 is unchanged so nothing catches it
    earlier. The blast radius is bounded by the record itself: it is keyed to
    runtime_incarnation, so only the exact same incarnation reads it back, and today no caller
    outside this module's tests reads it. Stated, not solved.
  • The server→client approval requests are still on the floor. The ten requestApproval /
    requestUserInput / elicitation methods still fall through observe's _ => return Ok(false). activeFlags says that an agent is blocked; those say what on, and pairing them
    with serverRequest/resolved is what would make blocked dwell time measurable. Not in this PR.

Related

Ready for review.

@schickling schickling added type:bug Something broken or a regression · Set: manual area:driver Harness drivers: launch, MCP, app-server, native delivery · Set: manual area:ding DING delivery: inbox notice into a running agent · Set: manual harness:codex Codex-specific behavior · Set: manual origin:agent Filed or primarily produced by an AI agent · Set: manual labels Aug 18, 2026
@schickling
schickling marked this pull request as ready for review August 18, 2026 06:57
@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, add credits to your account and enable them for code reviews in your settings.

`ThreadStatus`'s `active` arm carries a required `activeFlags` array, and
`observe_thread_status` took only the status word, discarding it at all
three call sites. Parse the field, map a known human-blocking flag onto an
explicit hold, and release the same turn when the flag clears.

Closes #265

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@schickling
schickling force-pushed the schickling/fix-265-active-flags branch from fe7a33d to 04e00b8 Compare August 18, 2026 07:07
@schickling

Copy link
Copy Markdown
Contributor Author

Rebased onto main, plus one integration change worth reviewing

main moved while this was open: #270 and #272 merged. Rebased onto 02fbed8.

#270 turned observe_turn_completed into an exhaustive match with no catch-all, and this branch adds two new CodexHoldReason variants — so the rebase did not compile:

error[E0004]: non-exhaustive patterns: `Held { reason: WaitingOnApproval, .. }`
              and `Held { reason: WaitingOnUserInput, .. }` not covered
   --> src/codex_app_server.rs:881:31

That is #270's exhaustive match doing exactly what it was written to do — its own comment says it "stays exhaustive so a new observed state cannot silently arrive as a conflict it never was". Before #270, these two variants would have fallen through the old _ arm and been silently reported as ConflictingTurn.

Resolution: both variants were added to the preserved arm, following #270's stated rule — "Only the signal that minted the hold releases it." The waiting-on-human holds are minted from activeFlags on a thread status, so they are cleared by the next thread status that omits the flag, not by a turn completing. The comment above the arm was extended to say so.

This supersedes the note in the original PR description that a turn completing while flagged "hits the pre-existing ConflictingTurn catch-all and is released by the next idle status" — that catch-all no longer exists, and the behaviour is now explicit rather than incidental.

Worth a reviewer's eye, because it is a behavioural decision made during integration rather than in the original change: a turn that completes while still flagged now stays held until a thread status clears the flag, instead of decaying to ConflictingTurn and waiting for idle. I believe that is the honest reading and it is consistent with how every other non-lifecycle hold is treated, but it is the one judgement here that was not in the reviewed change.

Verification after rebase: cargo test --locked --lib codex_app_server → 36 passed, 0 failed. cargo test --locked --lib ding → 68 passed, 0 failed, covering the tests named in the Fail-closed observed native DING invariant row. Conflicts with #271 are gone; both branches now sit directly on main.

@schickling
schickling merged commit 88cbe02 into main Aug 18, 2026
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:ding DING delivery: inbox notice into a running agent · Set: manual area:driver Harness drivers: launch, MCP, app-server, native delivery · Set: manual harness:codex Codex-specific behavior · Set: manual origin:agent Filed or primarily produced by an AI agent · Set: manual type:bug Something broken or a regression · Set: manual

Projects

None yet

Development

Successfully merging this pull request may close these issues.

observe_thread_status takes only the status word, so a required ThreadStatus field is parsed and discarded at all three call sites

1 participant